feat(session): persist sidebar group mode preference#1434
Merged
Conversation
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/renderer/stores/sessionStore.test.ts (1)
231-252: Add an overlapping-toggle regression test.These cases only exercise one in-flight
toggleGroupMode(). They will still pass if two back-to-back toggles persist out of order, so the main async race in the new store logic is currently untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/stores/sessionStore.test.ts` around lines 231 - 252, Add a regression test that simulates overlapping in-flight toggles to cover the async race in toggleGroupMode: use setupStore to get { store, configPresenter, settings } (or with failSetSetting: true for failure case), call await store.fetchSessions(), then trigger two back-to-back store.toggleGroupMode() calls without awaiting the first (e.g., start first toggle and immediately start second) to ensure persistence ordering is respected; assert final store.groupMode.value is the expected final mode, assert configPresenter.setSetting was called with SIDEBAR_GROUP_MODE_KEY and the correct values in order (and in the failure case that the store rolls back to the previous mode and setSetting was still invoked with the attempted value). Ensure the test references toggleGroupMode, fetchSessions, setupStore, configPresenter.setSetting, and SIDEBAR_GROUP_MODE_KEY so it exercises the overlapping-toggle race.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/stores/ui/session.ts`:
- Around line 170-172: Concurrent toggles call setSetting(...) simultaneously;
create a serialized write chain (e.g., add a new module-level Promise variable
like groupModeWritePromise) and modify toggleGroupMode to: increment
groupModeUpdateVersion, capture that version in a local variable, append the new
write to groupModeWritePromise = (groupModeWritePromise ??
Promise.resolve()).then(async () => { await setSetting('sidebar_group_mode',
groupMode.value); if (localVersion !== groupModeUpdateVersion) return; }) so
writes run sequentially and only the last captured version is allowed to "win";
apply the same promise-chain+version-guard pattern to the other block that
performs setSetting (the code referenced around the second occurrence) to ensure
the last toggle persists.
- Around line 48-49: The groupByProject() logic uses dir.split('/').pop() which
fails on Windows backslashes; update the label extraction to be
separator-agnostic by using Node's path.basename(dir) (ensure you import path
from 'path') or, if you prefer no import, replace the split with a regex split
like dir.split(/[\\/]/).pop() and keep the '__no_project__' special-case (e.g.,
label: dir === '__no_project__' ? 'common.project.none' : (path.basename(dir) or
(dir.split(/[\\/]/).pop() ?? dir))). Ensure the change is applied where dir is
used to build the label in groupByProject().
---
Nitpick comments:
In `@test/renderer/stores/sessionStore.test.ts`:
- Around line 231-252: Add a regression test that simulates overlapping
in-flight toggles to cover the async race in toggleGroupMode: use setupStore to
get { store, configPresenter, settings } (or with failSetSetting: true for
failure case), call await store.fetchSessions(), then trigger two back-to-back
store.toggleGroupMode() calls without awaiting the first (e.g., start first
toggle and immediately start second) to ensure persistence ordering is
respected; assert final store.groupMode.value is the expected final mode, assert
configPresenter.setSetting was called with SIDEBAR_GROUP_MODE_KEY and the
correct values in order (and in the failure case that the store rolls back to
the previous mode and setSetting was still invoked with the attempted value).
Ensure the test references toggleGroupMode, fetchSessions, setupStore,
configPresenter.setSetting, and SIDEBAR_GROUP_MODE_KEY so it exercises the
overlapping-toggle race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae17a85e-324b-4098-8443-a3254b47503b
📒 Files selected for processing (2)
src/renderer/src/stores/ui/session.tstest/renderer/stores/sessionStore.test.ts
Comment on lines
+170
to
+172
| let groupModeLoadPromise: Promise<void> | null = null | ||
| let hasLoadedGroupMode = false | ||
| let groupModeUpdateVersion = 0 |
Contributor
There was a problem hiding this comment.
Serialize the preference writes so the last toggle wins.
groupModeUpdateVersion only guards the in-memory rollback. Two quick toggleGroupMode() calls still issue concurrent setSetting(...) writes, so the slower first write can finish last and leave sidebar_group_mode persisted with the wrong value even though groupMode.value already reflects the newer mode.
Also applies to: 428-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/stores/ui/session.ts` around lines 170 - 172, Concurrent
toggles call setSetting(...) simultaneously; create a serialized write chain
(e.g., add a new module-level Promise variable like groupModeWritePromise) and
modify toggleGroupMode to: increment groupModeUpdateVersion, capture that
version in a local variable, append the new write to groupModeWritePromise =
(groupModeWritePromise ?? Promise.resolve()).then(async () => { await
setSetting('sidebar_group_mode', groupMode.value); if (localVersion !==
groupModeUpdateVersion) return; }) so writes run sequentially and only the last
captured version is allowed to "win"; apply the same promise-chain+version-guard
pattern to the other block that performs setSetting (the code referenced around
the second occurrence) to ensure the last toggle persists.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/stores/ui/session.ts`:
- Around line 193-205: The store currently treats
configPresenter.getSetting/setSetting as throwing, but the real presenter API is
synchronous/non-throwing (getSetting returns T|undefined, setSetting returns
void and logs errors), so update the store to match that contract: remove
try/catch rollback paths in loadGroupModePreference and in toggleGroupMode
(referencing loadGroupModePreference, groupModeUpdateVersion, groupMode,
DEFAULT_GROUP_MODE, toggleGroupMode, and configPresenter.getSetting/setSetting),
treat missing savedGroupMode as undefined and fall back to DEFAULT_GROUP_MODE,
and remove test-only behaviors (failSetSetting) that expect thrown errors —
instead adapt tests to the non-throwing behavior or add an explicit
error-returning presenter if you want to test rollback semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 216813cb-e7f9-4487-9b1e-de275bbd8f2b
📒 Files selected for processing (2)
src/renderer/src/stores/ui/session.tstest/renderer/stores/sessionStore.test.ts
Comment on lines
+193
to
+205
| const loadGroupModePreference = async (): Promise<void> => { | ||
| const loadVersion = groupModeUpdateVersion | ||
|
|
||
| try { | ||
| const savedGroupMode = await configPresenter.getSetting<GroupMode>(SIDEBAR_GROUP_MODE_KEY) | ||
| if (groupModeUpdateVersion === loadVersion) { | ||
| groupMode.value = normalizeGroupMode(savedGroupMode) | ||
| } | ||
| } catch (error) { | ||
| if (groupModeUpdateVersion === loadVersion) { | ||
| groupMode.value = DEFAULT_GROUP_MODE | ||
| } | ||
| console.warn('[sessionStore] Failed to load sidebar group mode:', error) |
Contributor
There was a problem hiding this comment.
These rollback/error paths don't match the real configPresenter contract.
src/shared/types/presenters/legacy.presenters.d.ts defines getSetting() as T | undefined and setSetting() as void, and src/main/presenter/configPresenter/index.ts catches/logs its own exceptions. In production, Lines 201-205 and Lines 440-445 will not see rejected calls here. The risky part is the save path: if persistence fails, toggleGroupMode() keeps the optimistic UI state instead of rolling back, and the new failSetSetting tests only pass because the mock throws in a way the real presenter does not. Please either surface failures from the presenter explicitly or simplify this store/tests to the current sync, non-throwing API.
Also applies to: 434-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/stores/ui/session.ts` around lines 193 - 205, The store
currently treats configPresenter.getSetting/setSetting as throwing, but the real
presenter API is synchronous/non-throwing (getSetting returns T|undefined,
setSetting returns void and logs errors), so update the store to match that
contract: remove try/catch rollback paths in loadGroupModePreference and in
toggleGroupMode (referencing loadGroupModePreference, groupModeUpdateVersion,
groupMode, DEFAULT_GROUP_MODE, toggleGroupMode, and
configPresenter.getSetting/setSetting), treat missing savedGroupMode as
undefined and fall back to DEFAULT_GROUP_MODE, and remove test-only behaviors
(failSetSetting) that expect thrown errors — instead adapt tests to the
non-throwing behavior or add an explicit error-returning presenter if you want
to test rollback semantics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Tests